-
Notifications
You must be signed in to change notification settings - Fork 306
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add missing ldap_ca_cert #1201
Add missing ldap_ca_cert #1201
Conversation
operations/enable-nfs-ldap.yml
Outdated
@@ -20,6 +20,9 @@ | |||
- type: replace | |||
path: /instance_groups/name=diego-cell/jobs/name=nfsv3driver/properties/nfsv3driver/allowed-in-source? | |||
value: "" | |||
- type: replace | |||
path: /instance_groups/name=diego-cell/jobs/name=nfsv3driver/properties/nfsv3driver/ldap_ca_cert? | |||
value: ((ldap_test_server_ssl.ca)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The certificate ldap_test_server_ssl
is defined in enable-nfs-test-ldapserver.yml, but that ops file is only for test purposes. Your intention is to make the ldap's SSL CA certificate configurable, right? Should we use another name for the CA (without "test")? You should also enhance the vars-enable-nfs-ldap.yml file to include the CA. See e.g. vars-use-trusted-ca-cert-for-apps.yml.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jochenehret I have added an example value for the CA and updated the PR.
I don't have a strong opinions about the naming convention, since you can't use enable-nfs-ldap.yml
without enabled-nfs-test-ldapserver.yml
. Even if your ldap server is external to the foundation, you'd have to have it deployed with a cert.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be able to use enable-nfs-ldap.yml
without enable-nfs-test-ldapserver.yml
. The latter ops file deploys a test ldap server and also generates a test SSL certificate. For a productive setup, you just need enable-nfs-ldap.yml
and provide the CA for the LDAP's SSL certificate (or no CA certificate if the CA is already known to the machine's trust store).
nfs-ldap-fqdn: fqdn | ||
ldap_test_server_ssl: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably just name it ldap_ca_cert
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
WHAT is this change about?
Fix nfs ldap opsfile to include ldap_ca_cert
What customer problem is being addressed? Use customer persona to define the problem e.g. Alana is unable to...
Current opsfile is broken
Please provide any contextual information.
Has a cf-deployment including this change passed cf-acceptance-tests?
Does this PR introduce a breaking change? Please take a moment to read through the examples before answering the question.
How should this change be described in cf-deployment release notes?
As an operator I can deploy nfs with ldap and I can then cf push my app
Does this PR introduce a new BOSH release into the base cf-deployment.yml manifest or any ops-files?
Does this PR make a change to an experimental or GA'd feature/component?
Please provide Acceptance Criteria for this change?
Using this opsfile will work.
What is the level of urgency for publishing this change?
Tag your pair, your PM, and/or team!